Skip to content

fix: remove default values in DTOs that cause value overwrite on update #1802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

martin-trajanovski
Copy link
Collaborator

@martin-trajanovski martin-trajanovski commented Mar 28, 2025

Description

This PR aims to fix the size and numberOfFiles fields overwrite in the partial update of a dataset.

Motivation

size and numberOfFiles fields were overwritten every time when a partial update on a dataset was performed.

Fixes

Changes:

  • changes made

Tests included

  • Included for each change/fix?
  • Passing?

Documentation

  • swagger documentation updated (required for API changes)
  • official documentation updated

official documentation info

@martin-trajanovski martin-trajanovski marked this pull request as ready for review March 31, 2025 09:59
@HayenNico
Copy link
Member

Small addition: This will also fix #1688 as it's caused by the same default values. I think we should still consider removing size as a DTO field for Dataset, since it's mostly managed/updated by the backend when changing OrigDatablocks.

@nitrosx
Copy link
Member

nitrosx commented Apr 1, 2025

@HayenNico should I bring it up in the collaboration meeting and ask opinion there?

That said, I think that this field is important given that one dataset can have multiple origDatablock and multiple files associated with it

@HayenNico
Copy link
Member

HayenNico commented Apr 1, 2025

@nitrosx I did not mean to remove size from Dataset schema, only the DTOs. In the code, when an OrigDatablock is created/updated, the size of the associated Dataset is also recalculated. As such, I'd call that a field that's managed by the backend, and it should not be available to edit in the Dataset DTO itself.
Right now, one could in principle overwrite size in a Dataset via the API, such that it does not have any relation to the data files anymore. See also #1688 for more details

@martin-trajanovski
Copy link
Collaborator Author

martin-trajanovski commented Apr 4, 2025

@nitrosx I did not mean to remove size from Dataset schema, only the DTOs. In the code, when an OrigDatablock is created/updated, the size of the associated Dataset is also recalculated. As such, I'd call that a field that's managed by the backend, and it should not be available to edit in the Dataset DTO itself. Right now, one could in principle overwrite size in a Dataset via the API, such that it does not have any relation to the data files anymore. See also #1688 for more details

Yes I agree with this. I think as we are calculating this on the backend side it is a good idea to remove it from the update and create DTOs and just leave it in the database schema. This removes the possibility to update those fields by mistake and have the backend calculations as the only source of truth when it comes to total size and number of files. I am not sure if I should do that as part of this PR or it can be done separately in another one. @nitrosx let me know what do you think

@nitrosx
Copy link
Member

nitrosx commented Apr 15, 2025

Remove size from the create and update DTO and manage it internally.
It should stay in the output DTO.

I would say create an additional PR

@nitrosx nitrosx merged commit 3bb795a into master Apr 23, 2025
13 checks passed
@nitrosx nitrosx deleted the SWAP-4573-remove-default-values-in-dtos-to-prevent-overwriting branch April 23, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants